Skip to content

Conversation

@jquense
Copy link
Member

@jquense jquense commented Mar 10, 2020

No description provided.

@jquense jquense requested a review from taion March 10, 2020 20:44
@@ -0,0 +1,69 @@
import useEffect from './useIsomorphicEffect'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import useEffect from './useIsomorphicEffect'
import useIsomorphicEffect from './useIsomorphicEffect'

maybe? otherwise it's less clear what kind of thing we're doing

Comment on lines 28 to 36
const {
attributeFilter,
attributeOldValue,
attributes,
characterData,
characterDataOldValue,
childList,
subtree,
} = config
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there really no good way to do shallow equality here? seems awkward to destructure config just to pass it back in again "re-structed", as it were

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope :/ could wrap it into a hook but it adds an extra layer of execution that seems worse

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i feel like it'd be a useful hook! we'd use it in react-relay-mutation at least...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added!

jquense added 2 commits March 12, 2020 13:45
An effect hook with customizable equality checks
@jquense
Copy link
Member Author

jquense commented Mar 12, 2020

@taion added a few things and made this more robust

@jquense jquense requested a review from taion March 12, 2020 17:47
depsRef.current = dependencies

if (!prev || !isEqual(dependencies, prev)) {
return effect()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arg i need to handle the teardown manually i think this will fire too often

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, what you want to do is:

image

const dependenciesRef = useRef();
dependenciesRef.current = dependencies;

const cleanupRef = useRef(null);

useEffect(() => {
  if (cleanupRef.current === null) {
    const cleanup = effect();

    cleanupRef.current = () => {
      if (isEqual(dependenciesRef.current, dependencies)) {
        return;
      }

      cleanupRef.current = null;
      cleanup();
    };
  }

  return cleanupRef.current;
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. dependenciesRef.current there is already the new value, while dependencies as bound in cleanupRef.current is still the old value

Copy link
Member Author

@jquense jquense Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm i see what you did, very clever

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i think it's the only way to do this without fully emulating everything ourselves

though, it's probably a good idea to pass in the "raw" dependencies list to the base effect fn

Comment on lines 33 to 37
function useCustomEffect<TDeps extends DependencyList = DependencyList>(
effect: EffectCallback,
dependencies: DependencyList,
options: CustomEffectOptions<TDeps>,
): void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the point of this overload? it looks like you're parametrizing on TDeps in all cases. so i don't see what could satisfy this overload but not e.g. the main fn defn below?

depsRef.current = dependencies

if (!prev || !isEqual(dependencies, prev)) {
return effect()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, what you want to do is:

image

const dependenciesRef = useRef();
dependenciesRef.current = dependencies;

const cleanupRef = useRef(null);

useEffect(() => {
  if (cleanupRef.current === null) {
    const cleanup = effect();

    cleanupRef.current = () => {
      if (isEqual(dependenciesRef.current, dependencies)) {
        return;
      }

      cleanupRef.current = null;
      cleanup();
    };
  }

  return cleanupRef.current;
});

type Deps = [Element | null | undefined, MutationObserverInit]

const isDepsEqual: IsEqual<Deps> = (prev, next) =>
prev[0] === next[0] && isEqual(prev[1], next[1])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you probably want "shallow equal" for the config.

actually we probably want a helper that does "shallow equal for each member of deps array"

depsRef.current = dependencies

if (!prev || !isEqual(dependencies, prev)) {
return effect()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. dependenciesRef.current there is already the new value, while dependencies as bound in cleanupRef.current is still the old value

* @param dependencies A list of dependencies
* @param isEqual A function comparing the next and previous dependencyLists
*/
function useCustomEffect<TDeps extends DependencyList = DependencyList>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this typing seems odd to me. DependencyList is typed as ReadonlyArray<any>... but realistically we're going to have something like a tuple type. do tuple types extend read-only arrays of type any?

... apparently so, but it's weird

function useCustomEffect<TDeps extends DependencyList = DependencyList>(
effect: EffectCallback,
dependencies: TDeps,
isEqualOrOptions: IsEqual<TDeps> | CustomEffectOptions<TDeps>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why the overloads are needed at all... seems like anything that would pass validation for the overloads would pass validation here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overloads here are mostly for documentation not safety, yes this would be fine by itself but i think the intellisense is usually nicer to read with overloads

Copy link
Member

@taion taion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, fair enough. looks good other than the noted issues

useMountEffect(() => {
if (!element) return

observerRef.current = new MutationObserver(fn)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
observerRef.current = new MutationObserver(fn)
observerRef.current = new MutationObserver(fn)
observer.observe(element, config)

don't you need this here? not that there can really be an element on immediate mount

jquense and others added 3 commits March 18, 2020 14:55
Co-Authored-By: Jimmy Jia <tesrin@gmail.com>
Co-Authored-By: Jimmy Jia <tesrin@gmail.com>
@jquense jquense merged commit 75cd372 into master Mar 18, 2020
@jquense jquense deleted the mitation branch March 18, 2020 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants